-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOCSP-34044: Add bitwise operators to aggregation pipeline #253
Conversation
✅ Deploy Preview for mongodb-docs-csharp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a first batch of comments so that you can propagate these changes to the other sections. Please lmk if you have questions baout any of my feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few more small suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job!! just a few nits
// start-bitAnd-collection-example | ||
|
||
var query = queryableCollection | ||
.Select(i => i.Price & i.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an odd example. There's no real reason to bit-and a Price
and a Count
.
A better example would involve flags or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the example!
source/fundamentals/linq.txt
Outdated
.. note:: Missing or Undefined Operands | ||
|
||
If the operand list you pass to any bitwise operator contains a missing or | ||
undefined value, the entire expression evaluates to ``null``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true server-side, but in C# this isn't true unless you are using "nullable" integers.
In these examples if either Price
or Count
is null
the query will fail client-side while deserializing the results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified the note, please take a look!
|
||
The ``null`` result comes from the document where the ``Name`` field | ||
has the value of ``"cheese"``. This document is missing a ``Count`` field, so | ||
the expression evaluates to ``null``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried to execute this?
I think it will throw an exception because the null
can't be deserialized to an int
client-side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All examples in this PR have been executed - although, the issue is that I was using nullable integers but did not add that in the example. They have now been added.
source/fundamentals/linq.txt
Outdated
{ "_id": 3, "name": "eggs", "price": 5, "count": 12 }, | ||
{ "_id": 4, "name": "potatoes", "price": 3, "count": 0 }, | ||
{ "_id": 5, "name": "pasta", "price": 4, "count": 100 }, | ||
{ "_id": 6, "name": "cheese", "price": 4 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This final document can't have been created using the Ingredient
POCO because Count
is an int
and can't be null
.
Also, the element names in these sample documents don't match the field names in the POCO (the C# properties start with a capital letter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data types used in the example have been changed to be nullable.
As for the second point, for these examples we use a convention pack (see the bottom of the Overview section on this page) to convert snake case field names into Pascal case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to tell from looking at just this PR that there is a convention somewhere else that is mapping the property names. That's confusing.
-13 | ||
-5 | ||
-101 | ||
null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
is not a possible result. It should throw an exception when deserializing the results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
* changing code + checkpint * very preliminary first draft * correct code block * code blocks * and and or * xor, not, and other small tweaks * changing variable type in linq.cs * never mind * back to double * addressing basic comments * move code to include * io code block * reworking first sentence * bullet list * bullet list pt 2 * bullet list pt 3 * feedback * nits * change example * correction * snake case * refining note * vale (cherry picked from commit a1b1f4c)
Pull Request Info
PR Reviewing Guidelines
JIRA - https://jira.mongodb.org/browse/DOCSP-34044
Staging Links
Self-Review Checklist